Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elevator #11

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Elevator #11

wants to merge 34 commits into from

Conversation

janetmeng
Copy link
Contributor

No description provided.

25laurenj and others added 30 commits January 14, 2023 12:42
public static final double TICKS_PER_INCH = TICKS_PER_REV*GEAR_RATIO/WHEEL_DIAMETER/Math.PI; //talonfx drive encoder
public static final double ELEVATOR_GEAR_RATIO = 25.0; // 25 motor spins for 1 shaft spin

public static final double TICKS_PER_INCH = (TICKS_PER_REV*ELEVATOR_GEAR_RATIO)/(ELEVATOR_SPROCKET_DIAMETER*Math.PI); //talonfx drive encoder
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to specify in the variable name that this is ticks per inch for the elevator. Otherwise we might accidentally use this for one of the other subsystems.

Comment on lines +25 to +33
/*public class Robot extends TimedRobot {
private static final String kDefaultAuto = "Default";
private static final String kCustomAuto = "My Auto";
private String m_autoSelected;
private final SendableChooser<String> m_chooser = new SendableChooser<>();
//private final AutonomousBase autonomousBasePD = new AutonomousBasePD(new Pose2d(0*Constants.TICKS_PER_INCH, -20*Constants.TICKS_PER_INCH, new Rotation2d(0)), 0.0, new Pose2d(), 0.0);
public static final DrivetrainSubsystem m_drivetrainSubsystem = new DrivetrainSubsystem();

*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out? Can we delete it?

@@ -1,146 +0,0 @@
package frc.robot.autonomous;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double checking that it was intentional to delete this file

new Translation2d(1.2, 0.0),
new Translation2d(1.4, 0.0)
);
// public static Trajectory uno = AutonomousBaseMP.generateTrajectory(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out? Is it needed anymore?

VideoSink server;
/*int aprilTagIds[];

float decisionMargin[];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete any of these that aren't needed anymore instead of commenting them out

// desiredTicks = (desiredInches-2) * Constants.OVER_TWO_TICKS_PER_INCH;
// }
// }
public void telescopeDeadband(){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be cleaner if this method took desiredTicks as an argument instead of having it as a class variable.

double milliTime = time * 1000;
System.out.println("milli time: " + milliTime);
if(forwards == true){
while(System.currentTimeMillis() - startTime <= milliTime){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While loops are scary and usually bad for robot code. This will block everything else until this amount of time has passed which will likely mess up other things. Instead this should be:
if(System.currentTimeMillis() - startTime <= milliTime) {
set(0.2)
} else {
set(0)
}

Comment on lines +326 to +339
public void scoreLow(){
elevatorSubsystem.setState(ElevatorStates.LOW_ELEVATOR_HEIGHT);
armTelescopingSubsystem.setTState(TelescopingStates.LOW_ARM_LENGTH);
}

public void scoreMid(){
elevatorSubsystem.setState(ElevatorStates.MID_ELEVATOR_HEIGHT);
armTelescopingSubsystem.setTState(TelescopingStates.MID_ARM_LENGTH);
}

public void scoreHigh(){
elevatorSubsystem.setState(ElevatorStates.HIGH_ELEVATOR_HEIGHT);
armTelescopingSubsystem.setTState(TelescopingStates.HIGH_ARM_LENGTH);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these belong in DrivetrainSubsystem? They seem unrelated to the drivetrain.

Comment on lines +16 to +28
public double _kP = 0.05;
public double _kI = 0.0;
public double _kD = 0.0;
public int _kIzone = 0;
public double _kPeakOutput = 1.0;

public static TalonFX elevatorMotor = new TalonFX(Constants.ELEVATOR_CAN_ID);
public static ElevatorStates elevatorState = ElevatorStates.LOW_ELEVATOR_HEIGHT;

// DigitalInput top_limit_switch = new DigitalInput(Constants.topLimitSwitchPort);
// DigitalInput bottom_limit_switch = new DigitalInput(Constants.bottomLimitSwitchPort);

public Gains elevatorGains = new Gains(_kP, _kI, _kD, _kIzone, _kPeakOutput);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can all of these be private?

Comment on lines +46 to +48
elevatorMotor.config_kP(Constants.kPIDLoopIdx, elevatorGains.kP, Constants.kTimeoutMs);
elevatorMotor.config_kI(Constants.kPIDLoopIdx, elevatorGains.kI, Constants.kTimeoutMs);
elevatorMotor.config_kD(Constants.kPIDLoopIdx, elevatorGains.kD, Constants.kTimeoutMs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thing but why are these lines indented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants